-
Notifications
You must be signed in to change notification settings - Fork 206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(cosmic-swingset): add swingset 'crankhash' to state vector #3626
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in principle. I presume the crankhash is a rolling hash so that sampling it just once at the end of the block is useful?
Right. Each crank's DB changes are (non-confusably) concatenated and hashed into the "crankhash" for that one crank, then the DB This includes the |
bd50ff5
to
1a70779
Compare
And in the specific.. am I writing the hash into the cosmos (consensus-checked) state vector correctly? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to change the crankhash
property to be named value
instead.
1288d9d
to
ef3723e
Compare
11ed6f0
to
5b4aff6
Compare
5b4aff6
to
ee2a0da
Compare
ee2a0da
to
cff858f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please consider my question, though.
@@ -232,6 +233,9 @@ export async function launch( | |||
blockHeight, | |||
blockTime, | |||
}); | |||
if (setActivityhash) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why is this not a part of the above bootstrapBlock
function as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, good question, I hadn't thought about it. I think that'd be fine, it would capture/signal any divergence during the initial controller.run()
that sets up the javascript world. Without it, we'd signal that divergence during the second block instead of during the first. The activityhash is updated for every commitCrank
(including the one that initializeKernel()
does, making its starting value effectively a hash of the config object), so even if we don't read it at that point, it's still sensitive to the initial state.
I'll add that.
This 'activityhash' covers all kernel state changes, so if any two validators diverge, their activityhashes should diverge too. By storing them into the cosmos-sdk state vector, the AppHashes will diverage as well, causing at least one of them to fall out of consensus. This should let us catch divergence as soon as possible. closes #3442
976541f
to
a8d2412
Compare
This 'crankhash' covers all kernel state changes, so if any two validators
diverge, their crankhashes should diverge too. By storing them into the
cosmos-sdk state vector, the AppHashes will diverage as well, causing at
least one of them to fall out of consensus. This should let us catch
divergence as soon as possible.
closes #3442